feat(collections): implement IDictionary<TKey,TValue> on the dictionary family - #338
Conversation
…ry family The set family got ISet<T> in 2.2.0; the dictionaries had the inverse gap — IReadOnlyDictionary<,> but not the mutable interface. IDictionary<,> does not derive from IReadOnlyDictionary<,>, so passing any Celerity dictionary except BTreeDictionary to an existing API taking IDictionary<,> was a hard compile error: the Guiding Principle #3 drop-in defect, one level up. Nine types now declare IDictionary<TKey, TValue?> alongside the read-only interface: CelerityDictionary, SwissDictionary, RobinHoodDictionary, HashCachingDictionary, PooledCelerityDictionary, SmallDictionary, IntDictionary, LongDictionary and EnumMap. Every member is an explicit-interface forwarder onto the existing public surface, so no existing public signature moved and the concrete indexer still returns the non-nullable TValue. Two calls worth recording: * The KeyCollection / ValueCollection struct views were widened from IEnumerable<T> to ICollection<T> rather than boxed into a fresh adapter type. dict.Keys stays allocation-free on the direct path while IDictionary<,>.Keys hands back a read-only ICollection<TKey> whose Add / Clear / Remove throw NotSupportedException, exactly as Dictionary<,>.KeyCollection does. CopyTo, Contains and IsReadOnly are public on the views, and CopyTo(KeyValuePair[], int) is public on the dictionaries, matching BTreeDictionary. * EnumMap was kept in rather than left out for its bounded key universe. An out-of-range enum cast is rejected with ArgumentOutOfRangeException, which is an ArgumentException — the failure IDictionary<,>.Add already documents for a key it cannot accept — so the implementation is honest rather than a member that throws where the contract says it should not. Documented on both surfaces and pinned by a test. Trie<TValue> is the one mutable one-value-per-key dictionary deliberately left out: its Keys / Values are lazy IEnumerable<T> traversals, not counted views, so widening them is a design change rather than a forwarder. FrozenCelerityDictionary is immutable, LruCache evicts on insert, and CelerityMultiMap is multi-valued; all three keep the read-only interface only. DictionaryInterfaceTests drives every member through the interface against a Dictionary<,> oracle, one row per dictionary (BTreeDictionary included, so the family contract lives in one place). Coverage holds at 100% line and branch. Closes #307
Coverage
|
…moke test Keys / Values are readonly structs reached through ICollection<T>, so the interface path boxes them and dispatches through an unboxing stub ILC has to generate. A foreach over the concrete type never compiles that path, so the smoke test now drives the whole interface surface — including the boxed views, their CopyTo, and the NotSupportedException from a view mutator — across the seven int-keyed dictionaries plus the long- and enum-keyed shapes.
There was a problem hiding this comment.
Pull request overview
This PR closes the drop-in parity gap for the dictionary family by adding the mutable BCL interface IDictionary<TKey, TValue?> (alongside the already-supported IReadOnlyDictionary<TKey, TValue?>) across the Celerity dictionary implementations, plus shared cross-collection contract tests and documentation updates.
Changes:
- Add
IDictionary<TKey, TValue?>to the mutable dictionary types via explicit-interface forwarders (keeping existing public signatures intact). - Widen
Keys/Valuesstruct views to implement read-onlyICollection<T>(mutators throwNotSupportedException) and add publicCopyTo(KeyValuePair<,>[], int)on the dictionaries. - Add
DictionaryInterfaceTeststo validateIDictionary<,>semantics across the family against a BCLDictionary<,>oracle; update docs/README/ROADMAP/CHANGELOG accordingly.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Celerity/Collections/CelerityDictionary.cs | Adds IDictionary<,> implementation + public CopyTo and read-only ICollection key/value views. |
| src/Celerity/Collections/SwissDictionary.cs | Same IDictionary<,> surface + view widening and CopyTo. |
| src/Celerity/Collections/RobinHoodDictionary.cs | Same IDictionary<,> surface + view widening and CopyTo. |
| src/Celerity/Collections/HashCachingDictionary.cs | Same IDictionary<,> surface + view widening and CopyTo. |
| src/Celerity/Collections/PooledCelerityDictionary.cs | Same IDictionary<,> surface + view widening and CopyTo, preserving disposed-state checks. |
| src/Celerity/Collections/SmallDictionary.cs | Same IDictionary<,> surface + view widening and CopyTo. |
| src/Celerity/Collections/IntDictionary.cs | Adds IDictionary<,> for int-keyed dictionary + view widening and CopyTo. |
| src/Celerity/Collections/LongDictionary.cs | Adds IDictionary<,> for long-keyed dictionary + view widening and CopyTo. |
| src/Celerity/Collections/EnumMap.cs | Adds IDictionary<,> with bounded-key behavior preserved; view widening and CopyTo. |
| src/Celerity.Tests/Collections/DictionaryInterfaceTests.cs | New shared suite asserting IDictionary<,> behavior across dictionary types (and includes BTreeDictionary). |
| docs/api/collections.md | Documents IDictionary<,> semantics and updates type declarations/notes across the affected dictionaries. |
| README.md | Updates public-facing interface support statements and guidance. |
| CHANGELOG.md | Adds [Unreleased] entries describing the new interface support, tests, and docs. |
| ROADMAP.md | Updates roadmap status for the dictionary-half of the interface parity work. |
…what it returns SumCountThroughInterface sums nothing — it returns Count through the interface-typed parameter. Rename it CountThroughInterface.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
CHANGELOG.md:12
- The new
[Unreleased]changelog entries are very long and implementation-detailed for this repo’s changelog conventions.CONTRIBUTING.md/CLAUDE.mdask for short, user-facing bullets (a few sentences at most) because the entire section becomes the GitHub Release body and can hit size limits. Consider condensing these four bullets to shorter summaries.
- **`IDictionary<TKey, TValue?>` on the mutable dictionary family** — `CelerityDictionary`, `SwissDictionary`, `RobinHoodDictionary`, `HashCachingDictionary`, `PooledCelerityDictionary`, `SmallDictionary`, `IntDictionary`, `LongDictionary` and `EnumMap` now implement the mutable BCL interface alongside `IReadOnlyDictionary<,>`, so passing one to an existing API taking `IDictionary<,>` compiles. `Keys` / `Values` widen to read-only `ICollection<T>` views whose mutators throw `NotSupportedException`, and `Contains` / `Remove` over a `KeyValuePair<,>` match on the pair rather than the key alone — both matching `Dictionary<,>`. Additive: no existing public signature changed, and `EnumMap`'s bounded key universe still rejects an out-of-range cast on the write surface. Closes [#307](https://github.com/marius-bughiu/Celerity/issues/307).
- A public `CopyTo(KeyValuePair<TKey, TValue?>[], int)` on each of those nine dictionaries, and `Contains` / `CopyTo` / `IsReadOnly` on their `KeyCollection` / `ValueCollection` views. Closes [#307](https://github.com/marius-bughiu/Celerity/issues/307).
- `DictionaryInterfaceTests` — a cross-collection suite driving every `IDictionary<,>` member through the interface against a `Dictionary<,>` oracle, one row per dictionary (`BTreeDictionary` included, so the family contract lives in one place), plus the bind-to-an-`IDictionary`-parameter case, `EnumMap`'s out-of-range key, and the disposed-`PooledCelerityDictionary` corner. Native AOT smoke coverage drives the same surface, since the boxed `Keys` / `Values` views are the one path a `foreach` over the concrete type never compiles. Closes [#307](https://github.com/marius-bughiu/Celerity/issues/307).
- Docs for the new interface: an `IDictionary<TKey, TValue?>` section in the API reference covering the semantics table and the read-only views, `EnumMap`'s bounded-universe caveat, and the updated README interface notes. Closes [#307](https://github.com/marius-bughiu/Celerity/issues/307).
…, user-facing form Addresses the Copilot review's suppressed CHANGELOG.md:12 comment: the four bullets read as an implementation walkthrough, which CONTRIBUTING.md and CLAUDE.md rule out — the whole section becomes the GitHub Release body and this repo has overrun the size cap before. The rollout is still reconstructible from one bullet per facet, just tersely.
|
Addressing the suppressed review comment on Agreed, and fixed in f96f5c0. The four bullets were an implementation walkthrough — the first one alone named all nine types, the view semantics, the pair-matching rule and the They now read as one tight line per rollout facet — interface, public Two other things from this round, for the record:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/Celerity/Collections/PooledCelerityDictionary.cs:912
ValueCollection.Count/IsReadOnlyaccess_dict._countwithout a disposed check. Like the key view, a previously captured values view can still be queried after the dictionary is disposed, which is inconsistent with the rest ofPooledCelerityDictionarythrowingObjectDisposedExceptionafter disposal.
public int Count => _dict._count;
/// <summary>Gets a value indicating whether the view is read-only. Always <c>true</c>.</summary>
public bool IsReadOnly => true;
src/Celerity/Collections/PooledCelerityDictionary.cs:813
KeyCollection.Count/IsReadOnlyread_dict._countwithout checking the disposed state. If a caller capturesvar keys = dict.Keys;and then disposes the dictionary,keys.Countstill succeeds even though other members consistently throwObjectDisposedException(and the outerCount/Keysproperties do). It’s safer and more consistent if the view properties also callThrowIfDisposed()before touching dictionary state.
This issue also appears on line 909 of the same file.
public int Count => _dict._count;
/// <summary>Gets a value indicating whether the view is read-only. Always <c>true</c>.</summary>
public bool IsReadOnly => true;
CHANGELOG.md:12
- The new
[Unreleased]bullets are much longer than the project’s changelog guidance (“a few sentences at most”, user-facing, no implementation walkthrough). This is called out inCONTRIBUTING.mdunder “Changelog entries”, and overly long sections are also a release risk because the workflow uses the section verbatim as the GitHub Release body.
- **`IDictionary<TKey, TValue?>` on the mutable dictionaries** — the nine hash-table and dense dictionaries now implement the mutable BCL interface alongside `IReadOnlyDictionary<,>`, so they drop into any API taking `IDictionary<,>`. `Keys` / `Values` widen to read-only `ICollection<T>` views, and `Contains` / `Remove` over a `KeyValuePair<,>` match on the pair — both as `Dictionary<,>` does. Additive: no existing public signature changed. Closes [#307](https://github.com/marius-bughiu/Celerity/issues/307).
- A public `CopyTo(KeyValuePair<TKey, TValue?>[], int)` on those dictionaries, plus `Contains` / `CopyTo` / `IsReadOnly` on their `Keys` / `Values` views. Closes [#307](https://github.com/marius-bughiu/Celerity/issues/307).
- `DictionaryInterfaceTests` — a cross-collection suite pinning the interface contract against a `Dictionary<,>` oracle, one row per dictionary, with matching Native AOT smoke coverage. Closes [#307](https://github.com/marius-bughiu/Celerity/issues/307).
- API-reference and README sections for the new interface, including `EnumMap`'s bounded-key caveat. Closes [#307](https://github.com/marius-bughiu/Celerity/issues/307).
…ve disposal The view structs hold a reference to the dictionary, so one captured while it was alive outlives it — the one way to reach the backing arrays after they have gone back to the pool. Contains and GetEnumerator already routed through members that throw, but Count read _dict._count directly and reported a stale number, and the CopyTo added in this PR would have raised a misleading ArgumentException from its space guard rather than reporting the real problem. Both views' Count and CopyTo now call ThrowIfDisposed first, closing the last hole in the "every member throws after dispose" contract #296 established for this type. IsReadOnly deliberately keeps answering: it is a constant that reads no dictionary state. Found by the Copilot review on #338.
|
Third review round — three suppressed comments, two accepted and one pushed back on. (Copilot posts these inside the review body rather than as inline threads, so there is nothing to resolve; replying here.) 1 & 2.
|
Benchmarks5 regressions Highlights
Collections (536)
Hashers (111)
Same-runner A/B (sharded 8-way): main ( |
Closes #307.
The set family got the mutable interface (
ISet<T>) in 2.2.0 and is getting the read-only one in #306. The dictionary family had the inverse gap:IReadOnlyDictionary<,>and notIDictionary<,>. SinceIDictionary<,>does not derive fromIReadOnlyDictionary<,>, passing any Celerity dictionary exceptBTreeDictionaryto an existing API takingIDictionary<,>was a hard compile error — the same Guiding Principle #3 drop-in defect, one level up.What changed
Collection (
src/Celerity/Collections/) — nine types now declareIDictionary<TKey, TValue?>alongsideIReadOnlyDictionary<TKey, TValue?>:CelerityDictionary,SwissDictionary,RobinHoodDictionary,HashCachingDictionary,PooledCelerityDictionary,SmallDictionary,IntDictionary,LongDictionary,EnumMap. Every member is an explicit-interface forwarder onto the existing public surface — no existing public signature moved, and the concrete indexer still returns the non-nullableTValue(IndexerReturnTypeTestspasses untouched).Semantics, all matching
Dictionary<,>:IsReadOnlyfalseAdd(TKey, TValue?)TryAddstays the non-throwing pathKeys/ValuesICollection<T>—Add/Clear/RemovethrowNotSupportedExceptionContains/Removeover aKeyValuePair<,>CopyToTwo design calls worth flagging for review:
The views were widened, not wrapped.
KeyCollection/ValueCollectionwent fromIEnumerable<T>toICollection<T>rather than getting a separate boxed adapter type. That keepsdict.Keysallocation-free on the direct path while the interface-typedKeysstill hands back a read-only collection.Contains,CopyToandIsReadOnlyare public on the views, andCopyTo(KeyValuePair<TKey, TValue?>[], int)is public on the dictionaries — mirroringBTreeDictionary, which already had both.EnumMapis in, not out. The issue flagged its bounded key universe as a possible reason to leave it out. An out-of-range enum cast is rejected withArgumentOutOfRangeException, which is anArgumentException— the failureIDictionary<,>.Addalready documents for a key it cannot accept — so this is an honest implementation, not a member that throws where the contract says it should not. Documented in the API reference and pinned byEnumMap_InterfaceAdd_ShouldRejectOutOfRangeKey.Deliberately excluded, stated in the test class doc and the ROADMAP:
FrozenCelerityDictionary(immutable),LruCache(evicts on insert, soAddcould silently drop an unrelated entry),CelerityMultiMap(many values per key), andTrie<TValue>— the one genuine one-value-per-key mutable dictionary left out, because itsKeys/Valuesare lazyIEnumerable<T>traversals rather than counted struct views, so widening them is a design change rather than a forwarder. Worth its own issue if wanted.Parity rollout
src/Celerity.Tests/Collections/DictionaryInterfaceTests.cs: the mirror ofReadOnlyDictionaryInterfaceTests. One row per dictionary drives the whole interface surface against aDictionary<,>oracle, plus the bind-to-an-IDictionary<,>-parameter case the issue reports as a compile error,EnumMap's out-of-range key, the disposed-PooledCelerityDictionarycorner, and the no-version-bump-on-overwrite rule through the interface indexer.BTreeDictionaryjoins the suite so the family contract is asserted in one place rather than drifting per type.AddAndTryAddTests,ContainsValueTests,IndexerReturnTypeTests,LoadFactorBoundaryTests,RemoveOutValueTests,ClearNoOpVersionTests, …) covers concrete members that did not change; all pass untouched.Celerity.AotSmokeTestnow drives the whole interface surface across the seven int-keyed dictionaries plus the long- and enum-keyed shapes. Worth having:Keys/Valuesare readonly structs reached throughICollection<T>, so the interface path boxes them and dispatches through an unboxing stub ILC has to generate — aforeachover the concrete type never compiles that path. All threeaot-publishlegs pass.Program.csregistration needed.COLLECTIONSentry inweb/dev/bench/index.html/detail.htmland no new ship card inweb/index.html.docs/api/collections.mdgains anIDictionary<TKey, TValue?>section (semantics table, read-only views, nullability note, runnable example) underCelerityDictionary, the declaration fences and peer prose updated for all nine types, and anEnumMapbounded-universe caveat.README.md: the "All dictionaries implement…" paragraph, the API-surface paragraph, and the stale "BTreeDictionary<,>also implements the mutableIDictionary<,>" sentence under "Choosing a collection".[Unreleased]→### Addedbullets (interface, publicCopyTo/ view members, tests, docs).donewith the two design calls; the set half staysin-progressbehind Implement IReadOnlySet<T> on the ten mutable sets #306.Test plan
dotnet build— clean, 0 warnings onCelerity.dllacrossnet8.0/net9.0/net10.0dotnet test— 5242 passed, 0 failed on all three TFMscoverage.runsettings+scripts/coverage_report.pyat the 100/100 floordotnet pack -c Release—EnablePackageValidationagainst the 2.4.0 baseline passes; the change is additive and binary-compatible🤖 Generated with Claude Code